Skip to content

Unit/functional test improvements to enable CTSM SystemTests #1426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Jul 21, 2025
Merged

Unit/functional test improvements to enable CTSM SystemTests #1426

merged 18 commits into from
Jul 21, 2025

Conversation

@samsrabin
Copy link
Contributor

@samsrabin samsrabin commented Jun 18, 2025 •
edited
Loading

Description:

  • Gets actual path to default config file to enable running from anywhere.
  • Adds an error check that would have helped in my troubleshooting.
  • Adds optional --config-file argument.
  • Adds netCDF file required for unit testing.

Previously filed as adrifoster#4.
CTSM PR here: ESCOMP/CTSM#3251

Waiting on NGEET GitHub org to get Git LFS working.

Collaborators:

Expectation of Answer Changes:

None

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

  • The in-code documentation has been updated with descriptive comments
  • The documentation has been assessed to determine if updates are necessary

Integrator

  • FATES PASS/FAIL regression tests were run
  • Evaluation of test results for answer changes was performed and results provided

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

samsrabin added 10 commits June 12, 2025 15:50
@samsrabin
run_unit_testing.py: Enable running from anywhere.
37a61ab
@samsrabin
run_unit_tests.py: Fail if config_file doesn't exist or is a dir.
7c61be7
@samsrabin
run_unit_tests.py: Add optional --config-file argument.
f35b05f
@samsrabin
Functional test fix: DATM file now found when using -r.
6cc3141
@samsrabin
Functional tests now work if called from somewhere other than testing/.
a2c2a2d
@samsrabin
Functional tests: Simplify an error.
c0af355
@samsrabin
run_functional_tests.py: Add optional --config-file argument.
08c1dbc
@samsrabin
Functional tests: Error if Fortran executable fails.
a80dd9e
@samsrabin
Functional tests: Fortran now errors instead of just stopping.
d34666c
@samsrabin
Functional tests: Download datm_file if needed. KLUDGE!
3b47323 
This is a kludge because it (a) downloads the datm_file from my Dropbox and (b) requires an extra line in each config file section. Revert this change once BONA_datm.nc is on Git LFS and that's working consistently.
@samsrabin samsrabin self-assigned this Jun 18, 2025
@samsrabin samsrabin added the type: testing Related to improvements in code testing (e.g. unit, regression, etc) label Jun 18, 2025
This was referenced Jun 18, 2025
Closed
Merged
adrifoster
adrifoster reviewed Jun 20, 2025
View reviewed changes
Copy link
Contributor

@adrifoster adrifoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I have some suggestions - namely I think we need a new extended base class to handle driver data instead of just adding it as a new argument to all classes

testing/utils.py
dictionary[section][option] = config.get(section, option)
value = config.get(section, option)

# If the option is one that we expect to be a path, ensure it's an absolute path.
Copy link
Contributor

@adrifoster adrifoster Jun 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like some of these updates could be moved into the check_arg_validity method. I think we could also combine your new get_abspath_from_config_file with the other file checking methods that are already here and just use it instead.

Copy link
Contributor Author

@samsrabin samsrabin Jun 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the config file check to check_arg_validity(). I'm not sure what you mean about get_abspath_from_config_file(), though?

@samsrabin samsrabin requested a review from adrifoster June 21, 2025 16:11
@glemieux
Copy link
Contributor

glemieux commented Jun 30, 2025 •
edited
Loading

@samsrabin samsrabin marked this pull request as ready for review July 1, 2025 16:56
@samsrabin
Copy link
Contributor Author

samsrabin commented Jul 1, 2025

@glemieux git-lfs worked for me on a fresh clone!

@adrifoster That means this is actually now ready for re-revew.

@adrifoster
Copy link
Contributor

adrifoster commented Jul 1, 2025

@glemieux git-lfs worked for me on a fresh clone!

@adrifoster That means this is actually now ready for re-revew.

great! I will take a look today

@glemieux glemieux moved this to Finding Reviewers in FATES Pull Request Planning and Status Jul 2, 2025
adrifoster
adrifoster approved these changes Jul 7, 2025
View reviewed changes
Copy link
Contributor

@adrifoster adrifoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these updates! Looks great

@samsrabin samsrabin mentioned this pull request Jul 14, 2025
Open
5 tasks
@glemieux glemieux moved this from Finding Reviewers to Final Testing in FATES Pull Request Planning and Status Jul 17, 2025
@glemieux glemieux mentioned this pull request Jul 21, 2025
Closed
@glemieux
Copy link
Contributor

glemieux commented Jul 21, 2025

I ran into #1441 during a test to simply make sure the run_unit_tests.py and run_functional_tests.py both run successfully. It looks like the issue may have been specific to my clone of ctsm and simply required removing the _build (and _run) directories to create a fresh build directory.

@glemieux glemieux merged commit 49cd7d0 into NGEET:main Jul 21, 2025
1 check was pending
@github-project-automation github-project-automation bot moved this from Final Testing to Ready to Integrate in FATES Pull Request Planning and Status Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adrifoster adrifoster adrifoster approved these changes

Assignees

@samsrabin samsrabin

Labels

type: testing Related to improvements in code testing (e.g. unit, regression, etc)

Projects

FATES Pull Request Planning and Status
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@samsrabin @glemieux @adrifoster